test(ingestion): cover newest-version selection in the Mlflow source#30198
Merged
Conversation
Fixes #30189 Select the newest registered model version by version number. Matching the parent's last_updated_timestamp only held while the version creation was the last write to the registered model; any later edit to its description, tags or aliases desynced the two and silently dropped the model. Point the Playwright Mlflow connection at sqlite backends. The placeholder strings resolved to MLflow's file store, which 3.13+ rejects, so the AutoPilot agent failed to connect and the run-completed banner never rendered.
search_registered_models returns a list of RegisteredModel, but the cast claimed the collection itself was one. Iterating a RegisteredModel yields its property pairs, so basedpyright inferred the loop variable as tuple[str, Any] and rejected every attribute access on it. The cast was a no-op at runtime. Modernize the annotations to PEP 585/604 while here, which retires the UP006, UP035 and UP045 suppressions rather than moving them.
Register a second model version with distinct hyperparameters and assert the ingested entity carries the newest version's run, not the older one's. Without this the fixture created a single version, so a matcher that selected a stale version would still pass. Extract the log-model retry into a helper shared by both versions.
github-actions
Bot
added
Ingestion
safe to test
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
Squash-merging #30191 and then merging main back into this branch duplicated the update_registered_model call, since git could not match the squashed block to the branch's copy. Remove the redundant second block.
Contributor
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically. Maintainers can bypass this check by adding the |
|
Contributor
🟡 Playwright Results — all passed (31 flaky)✅ 4540 passed · ❌ 0 failed · 🟡 31 flaky · ⏭️ 95 skipped
🟡 31 flaky test(s) (passed on retry)
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
pmbrull
approved these changes
Jul 20, 2026
Code Review ✅ Approved 1 resolved / 1 findingsAdds robust test coverage for MLflow version selection by registering multiple model versions and asserting hyperparameter ingestion. No issues found. ✅ 1 resolved✅ Quality: Duplicated update_registered_model call and comment
OptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #30191, addressing a Greptile review comment: the Mlflow integration test created a single model version, so
max(..., key=version)was never exercised — a matcher that selected a stale version would still pass.What
Register a second model version with distinct hyperparameters (
alpha=0.2, l1_ratio=0.3), then assert the ingested entity carries the newest version's run (alpha=0.5, l1_ratio=1.0) and that the older version's values are absent. The log-model retry loop is extracted into a shared helper so both versions register through the same path.Key finding
RegisteredModel.latest_versionsreturns a single element — the newest version — even with many versions registered (verified against MLflow 3.10 and 3.14: three versions still yield a one-element list). Stages were the only thing that produced multiple entries, and they are deprecated. So the connector'smax()cannot be discriminated bymin-vs-maxno matter how many versions exist. What the second version does change is which run the connector reads (get_run(latest_version.run_id)), so asserting the ingested hyperparameters match the newest run is the meaningful coverage — it catches a regression that walked all versions and grabbed the oldest.Verified locally against the testcontainers stack: the strengthened test fails with the pre-fix connector (
Version not found) and passes with #30191's fix.Note
This branch is stacked on #30191 and currently contains its commits too; the diff narrows to just the test once #30191 merges. Draft until then.
Greptile Summary
This PR strengthens the MLflow newest-version integration test. The main changes are:
Confidence Score: 5/5
This looks safe to merge.
Important Files Changed
Reviews (1): Last reviewed commit: "test(ingestion): drop describe block dup..." | Re-trigger Greptile
Context used: